lint ImproperCTypes: overhaul (take 2 of "better handling of indirections") #134697

niacdoial · 2024-12-23T17:59:31Z

This PR started as a try to re-add the changes of #131669 (reverted in #134064 after one (1) nightly)

It has since evolved in an overhaul of the ImproperCTypes lints, splitting the two lints into four:

one for "imported" items in extern "ABI" blocks
one for "exported" extern "ABI" functions
one for "exported" #[no_mangle]/1[export_name=_] static variables
one for extern "ABI" callbacks (ty::FnPtr arguments/return types/ADT fields)

It also allows for "detailed" lint messages, with successive notes that amount to "this is unsafe because of that, which is unsafe because of that, which is unsafe for this reason", and possible help messages at every step.

Hopefully the overall architecture of the lint is something less special-case-y, with better-separated abstractions.

the comment containing the most recent summary of the decisions and considerations is #134697 (comment)

oh, also, since this was already fixing it for the wrong reasons, I decided to add something that fixes it for the right ones:
Fixes #130310

(leaving this in because it was in the original version of this description:)
r? workingjubilee (because you reviewed the first attempt)

niacdoial · 2024-12-23T18:03:42Z

(
hi Jubilee, I'm back at this again!
I know this is not the best time of year to add PRs, so I'm fine with postponing this if you don't feel like tackling it these upcoming weeks.
In any case, have some nice end-of-year festivities, if you celebrate any!
)

niacdoial · 2024-12-23T18:08:24Z

ah, and before I forget: a small part of the new test file is commented out because it hits ICE #134587, but there should be more than decent coverage anyway

workingjubilee · 2024-12-24T09:16:24Z

unfortunately the lint needs to be gutted and rewritten.

workingjubilee · 2024-12-24T09:18:44Z

Also while I was possibly having a mild case of get-there-itis and thus mostly tried to just make sure things were coherent, I would prefer all new code for the lint be in compiler/rustc_lint/src/types/improper_ctypes.rs.

workingjubilee · 2025-01-04T18:24:23Z

The version cut happened so there will be less time pressure now.

niacdoial · 2025-01-05T00:29:14Z

I have a first version for compiler/rustc_lint/src/types/improper_ctypes.rs if you want.
it's less of a from-the-ground-up rewrite as it is scrapping the original for parts, if the analogy makes sense.

you probably have things to say about its architecture, even if the whole thing still have a bunch of TODO comments
the progress so far looks like this:

completely separate the type-checking and reporting systems
(part of the way there) remove some of the special cases and integrate them to the "main logic"
- check_for_opaque_types is still a "special case" part of the checking logic
- Cstr and Cstring are also somewhat special-cased because the advice for them depends on the type around them, if any
- the unit type is handled in multiple places, see if this can be fixed
(almost complete) compile, pass existing tests
- only failed tests are for Cstring, due to different error messages
- one unrelated test had to have a second "#[allow(improper_ctypes)]" added, but it makes more sense for it to need that anyway
better separation of the different checks in different visit_* methods of ImproperCTypesVisitor
better tracking of how the currently-checked type is used (static, function argument, function return's inner type, etc...)
- raises questions about the separation of improper_ctypes and improper_ctypes_definitions versus declared/defined functions, especially when FnPtr:s are involved
allow single argument check to emit multiple errors (for fnptr:s, structures with multiple FFI-unsafe fields, etc)
review what is considered FFI-safe or not (once everything else is complete)

If you want to take a look in this state, should I just commit it here? (possibly put the PR in draft mode while I'm at it?)
or send you the files in a different way?

bors · 2025-01-15T12:53:21Z

☔ The latest upstream changes (presumably #135525) made this pull request unmergeable. Please resolve the merge conflicts.

niacdoial · 2025-01-18T20:20:25Z

aaaand I think I have something that's "first draft" material! (should I put this pull in the "draft" state?)
There's still a bunch of ~~TODOs~~ (...well, they were changed to FIXMEs to be pushed here) for questions I didn't manage to answer (and a bunch of failing tests because I don't know if the error should be here), but yeah.

here's a list of some of my remaining questions and concerns:

visit_numeric seems too x86_64-specific
should we revisit the distinction between ImproperCTypes and ImproperCTypesDefinitions?
- part 1: the output ("external fn" or vs "external block" vs other possibilities)
- part 2: handling opaque types (there's a high correlation between ImproperCTypesDefinitions and places where we allow FFI-opaque types to be fully specified. do we want this correlation to be 1?)
more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built
- should we introduce a std::ffi::FfiOpaquePtr type? (which would be a *const c_void and some phantomdata, on first approximation)
for indirections whose values may be supplied by non-rust code: do we only allow pointers (and Optionstd::ptr::NonNull), or do we also allow Option<&T> and Option<Box<T>>?
not sure if the new error messages are intelligible in all cases (especially if there's a type param like Self or <Self as ::std::ops::Add<Self>>::Output that gets resolved in the error message).
it feels like the current handling of CStr/Cstring and Option-like enums uses special casing, since those are tested for in multiple places.
if we deny references and boxes in defined functions, what of &self in methods? We don't allow *const Self, last I checked.

bors · 2025-01-23T18:22:31Z

☔ The latest upstream changes (presumably #135921) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee · 2025-01-31T05:14:25Z

hmm.

workingjubilee · 2025-01-31T05:15:35Z

aaaand I think I have something that's "first draft" material! (should I put this pull in the "draft" state?)

Yes, it's a good marker for "I don't want this merged yet, even if it looks done".

workingjubilee · 2025-02-20T18:35:04Z

visit_numeric seems too x86_64-specific

It probably is.

should we revisit the distinction between ImproperCTypes and ImproperCTypesDefinitions?

Yes, but in particular, not to just repartition them between: I think breaking them into as many conceptually-smaller lints as possible is good, as long as each one is a distinct idea (no splitting just for the sake of splitting!).

more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built

I'm not sure what you mean?

for indirections whose values may be supplied by non-rust code: do we only allow pointers (and Option<std::ptr::NonNull>), or do we also allow Option<&T> and Option<Box<T>>?

We must allow Rust code to declare a pointer in a C signature to be Option<&T> or a number of things about our FFI story fall apart.

it feels like the current handling of CStr/CString and Option-like enums uses special casing, since those are tested for in multiple places.

Yes, probably.

workingjubilee

some initial nits on a first pass

compiler/rustc_lint/src/types/improper_ctypes.rs

workingjubilee · 2025-02-20T21:45:48Z

compiler/rustc_lint/src/types/improper_ctypes.rs

+                        // but for some reason one can just go and write function *pointers* like that:
+                        // `type Foo = extern "C" fn(::std::ffi::CStr);`


Because unsized function parameters are something we may want to support.

The code may not be well-formed: as you may have noticed at some point, you get warnings even if you get errors (usually), and this is because we lint even on "bad" code. This is because rustc didn't use to, once upon a time, and it was a bad debugging experience.

compiler/rustc_lint/src/types/improper_ctypes.rs

niacdoial · 2025-02-27T22:12:58Z

alright, sorry for taking a while!

I'm currently planning what changes I'll do in terms of splitting the lint(s)
my current idea is to separate based on the nature of the thing being (presumably) propped up against a FFI boundary

improper_ctypes: what's definitely an interface to an outside library (extern statics, extern function declarations)
improper_ctypes_fn_definitions: functions written in rust intended to be exported
improper_ctypes_callbacks: FnPtr arguments, no matter in what function they are being used
improper_ctypes_ty_definitions: repr(C) structs/enums(/unions)?

more on FFI-opaque types: how do we handle that in the context of the "context switch" between functions and possible FnPtr arguments? The answer that seems correct currently prevents a stage1 compiler from being built

I'm not sure what you mean?

well, this is more or less answered in what I said before that, but my question was about how to deal with "switching" from checking arguments for, say, a function definition, to checking the arguments of a FnPtr argument?

should the nature of the lint change?
(temptative answer: yes)
how should FFI-Safe-pointers-to-FFI-Unsafe-pointees work in FnPtr arguments? Should it be the rules for extern fn declarations? (throw the lint because one should use *const c_void, an extern type declaration, etc...) or the rules for extern fn definitions? (allow that, the function's body needs the full type even if it's opaque to the other side of the FFI boundary)
(temptative answer: it should be the former, but parts of the rustc codebase doesn't follow this rule, so I can't get a stage1 compiler if I make that the rule)

// you would think that int-range pattern types that exclude 0 would have Option layout optimisation
// they don't (see tests/ui/type/pattern_types/range_patterns.stderr)
// so there's no need to allow Option<pattern_type!(u32 in 1..)>.

oh, I should fix that probably

I... maybe? I can't for the life of me find the link to that again but I think I saw a discussion about that and type covariance/contravariance,
where i32 is 1.. is a subtype of i32 (well that was under consideration), meaning fn(Option<i32>) is a type of fn(Option<i32 is 1..>) and it might have impacts on whether there should be an optimisation because of transmutation?

Though you'll definitely know more than me on all the moving parts.
Especially assuming you might have looked at this more in the past week.

As for the rest of your advice, I already took all this in!
thanks for shedding light on my code, one nit at a time!

rustbot · 2025-05-30T23:37:41Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR modifies tests/auxiliary/minicore.rs.

cc @jieyouxu

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_codegen_llvm/src/llvm/enzyme_ffi.rs

cc @ZuseZ4

The Miri subtree was changed

cc @rust-lang/miri

niacdoial · 2025-05-30T23:39:43Z

...ok that's what I get for clicking a button at 1h30 am. I'll fix all this new stuff tomorrow

niacdoial · 2025-05-31T15:40:12Z

as an answer to those automated warnings:

things that might need further changes

clippy, rust-analyzer, miri:

mostly changes to add the new lints to the documentation, and some expected lint reshuffling in the tests
though, the fact that they are in subtrees could cause a problem.
Should I still move that piece of the PR on the individual repos, with some #[allow(unknown_lints)] trickery to ensure stuff doesn't break when they are merged prior to this?

things with no further changes needed IMO

rustc_codegen_cranelift

just a lint rename

This PR modifies tests/auxiliary/minicore.rs.

yes. I was removing #[allow(improper_ctypes_definitions)] to change to one of the new lints, but it turns out there is not need to #[allow()] anything

file in tests/crashes

done

Cargo.lock

the only change is an added dependency of rustc_lint on rustc_type_ir

compiler/rustc_codegen_llvm/src/llvm/enzyme_ffi.rs (and rustc_codegen_llvm in general)

the changes are basically to wrap the extern "C" { fn(_) -> &ReturnType } functions' return types into options, then actually .unwrap() this option, in order to change UB into proper panics

bors · 2025-05-31T21:59:53Z

☔ The latest upstream changes (presumably #141824) made this pull request unmergeable. Please resolve the merge conflicts.

niacdoial · 2025-05-31T22:09:38Z

at this point I think I'll deal with this (merge conflicts) when reviews have occurred. Probably the same amount of human work on my side, but it prevents changes of code mid-review, and will save a bunch of compute time running all tests every two days

workingjubilee · 2025-06-04T00:36:39Z

Sorry, had taken a bit to claw through review backlog accumulated during RustWeek!

workingjubilee · 2025-06-04T01:06:55Z

Uninhabited types are now deemed unsafe in function arguments and static variables. For function returns, only ! and empty enums are allowed. (e.g. structs with uninhabited fields are not)

So, an odd thing happened: We discovered that "a struct with uninhabited fields" is actually how we model the ABI of functions that are marked [[noreturn]] but nonetheless have a possibly-complex return type that would require e.g. allocation on the stack. So uninhabitedness should probably just be allowed, without linting, if the rest of the type is C-compatible? We can dial in from there.

Specifically, we fixed "#[repr(transparent)] types containing uninhabited 1-ZSTs as the non-transparent-wrapped field" so that they uphold the ABI-compatibily guarantee implied by #[repr(transparent)].

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

compiler/rustc_lint/src/types/improper_ctypes.rs

niacdoial · 2025-06-04T09:50:50Z

[...] So uninhabitedness should probably just be allowed, without linting, if the rest of the type is C-compatible? We can dial in from there.

Specifically, we fixed "#[repr(transparent)] types containing uninhabited 1-ZSTs as the non-transparent-wrapped field" so that they uphold the ABI-compatibily guarantee implied by #[repr(transparent)].

so... option 1 (all uninhabited returns allowed) for now, and later restrict this to option 2 (only !, EmptyEnum and #[repr(transparent)] structs with one of those as one of its 1-ZSTs)?

Sorry, had taken a bit to claw through review backlog accumulated during RustWeek!

oh, right I should have anticipated this. still, no problem! I kinda expected you to have a lot of other things taking your time (especially if some are more critical than a lint revamp)

niacdoial · 2025-06-04T14:43:59Z

Adding this pull that just landed and causes a few merge conflicts as required reading for myself, eventhough I started reading parts of it.

(I should have seen this while it was in RFC... at least this gives me a slight clarification about the PowerAlignment lint thing)
I guess I forgot about visit_numeric's x86_64-centricity in my summary comment. At least removing the u128/i128 lint altogether will make this a clean slate if/when it gets revisited to be architecture-dependant in the future.
(RalfJung, if you see this, I'm mostly sure the ImproperCTypes lints do in fact look into dependencies, so it should be fine to add something architecture-dependant. In any case, I'll double-check that statement if I do something about it)

bjorn3 · 2025-06-06T09:59:42Z

library/panic_abort/src/lib.rs

@@ -23,7 +23,8 @@ use core::any::Any;
 use core::panic::PanicPayload;

 #[rustc_std_internal_symbol]
-#[allow(improper_ctypes_definitions)]
+#[allow(unknown_lints, renamed_and_removed_lints, improper_ctypes_definitions)] // FIXME delete line once improper_c_fn_definitions exists upstream
+#[allow(improper_c_fn_definitions)]
 pub unsafe extern "C" fn __rust_panic_cleanup(_: *mut u8) -> *mut (dyn Any + Send + 'static) {


We should be able to use extern "Rust" I would think (both here and on the caller side).

OK!
I assume this is also due to something like the stage0 rework (something along the lines of "we now know caller and callee are compiled by the same compiler")?
Let's just see if it passes the tests then.

bjorn3 · 2025-06-06T10:00:08Z

library/panic_abort/src/lib.rs

@@ -23,7 +23,8 @@ use core::any::Any;
 use core::panic::PanicPayload;

 #[rustc_std_internal_symbol]
-#[allow(improper_ctypes_definitions)]
+#[allow(unknown_lints, renamed_and_removed_lints, improper_ctypes_definitions)] // FIXME delete line once improper_c_fn_definitions exists upstream


Should be possible to remove this now that the stage0 rework has been done: https://blog.rust-lang.org/inside-rust/2025/05/29/redesigning-the-initial-bootstrap-sequence/

done, let's see if it passes the tests (will resolve the conversation when it does)

bjorn3 · 2025-06-06T10:03:05Z

compiler/rustc_codegen_llvm/src/back/write.rs

@@ -462,10 +462,11 @@ fn report_inline_asm(
    cgcx.diag_emitter.inline_asm_error(span, msg, level, source);
 }

-unsafe extern "C" fn diagnostic_handler(info: &DiagnosticInfo, user: *mut c_void) {
+unsafe extern "C" fn diagnostic_handler(info: Option<&DiagnosticInfo>, user: *mut c_void) {


I disagree with having to wrap all references in Option. You have the invariant that the reference is valid (if not NULL) either way. It is very much reasonable to expect the C side not to pass NULL, in which case the argument will never be None.

ok!
I'll assume you have the same to say about the changes in compiler/rustc_llvm/src/lib.rs, library/proc_macro/src/bridge/{buffer,closure}.rs, and src/tools/clippy/tests/ui/boxed_local.rs, so I'll be also removing the new Option<_>s there as well.

workingjubilee · 2025-06-10T20:01:24Z

I opened #142310 to handle the power alignment lint problem.

- now the lint scans repr(C) struct/enum/union definitions - it now also scans method declarations in traits - many other changes in the underlying logic - some extra tests

- also fix a couple of thorny typos in/around types.rs::is_outer_optionlike_around_ty() and subsequently fix library and tests

- do not check ADT definitions themselves, it turns out `repr(C)` is not a strong enough signal to determine if something is designed for FFIs - however, start checking static variables with `#[no_mangle]` or `#[export_name=_]`, even if that's not a perfect signal due to the lack of specified ABI - some changes to the LLVM codegen so it can be seen as FFI-safe

for now, let's fully remove this lint. it might be reintroduced later as a way to make the lints ignore the inside of some ADT definitions

- make clippy happy about the changes in rust_codegen_llvm - split a test stderr into 32bit and 64bit - fix some documentation

it would be correct to lint on those, but this is deemed too steep a change for now, especially for projects that turn all warnings into errors

- relaxed the uninhabited types allowed on function return

rustbot assigned workingjubilee Dec 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2024

workingjubilee added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2025

niacdoial force-pushed the linting-ptrdyn-ffi branch from 5764b36 to 6c19cff Compare January 18, 2025 20:20

This comment has been minimized.

Sign in to view

niacdoial force-pushed the linting-ptrdyn-ffi branch from 6c19cff to 13720e7 Compare January 28, 2025 23:34

This comment has been minimized.

Sign in to view

workingjubilee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2025

workingjubilee self-requested a review January 31, 2025 05:14

workingjubilee marked this pull request as draft January 31, 2025 05:15

workingjubilee reviewed Feb 20, 2025

View reviewed changes

workingjubilee added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2025

workingjubilee reviewed Feb 21, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

niacdoial force-pushed the linting-ptrdyn-ffi branch from a32c27b to b70eb45 Compare May 30, 2025 21:16

niacdoial marked this pull request as ready for review May 30, 2025 23:37

workingjubilee reviewed Jun 4, 2025

View reviewed changes

compiler/rustc_codegen_llvm/src/llvm/ffi.rs Outdated Show resolved Hide resolved

workingjubilee reviewed Jun 4, 2025

View reviewed changes

compiler/rustc_lint/src/types/improper_ctypes.rs Show resolved Hide resolved

niacdoial force-pushed the linting-ptrdyn-ffi branch from afa32e8 to 2821fb5 Compare June 5, 2025 23:31

bjorn3 reviewed Jun 6, 2025

View reviewed changes

niacdoial added 10 commits June 11, 2025 01:29

lint ImproperCTypes: split the two lints into four total [...]

26c1428

- now the lint scans repr(C) struct/enum/union definitions - it now also scans method declarations in traits - many other changes in the underlying logic - some extra tests

lint ImproperCTypes: deal with uninhabited types

22db045

lint ImproperCTypes: redo handling of pattern types [...]

dab12c2

- also fix a couple of thorny typos in/around types.rs::is_outer_optionlike_around_ty() and subsequently fix library and tests

lint ImproperCTypes: rm improper_ctype_definitions [...]

a97b6ae

for now, let's fully remove this lint. it might be reintroduced later as a way to make the lints ignore the inside of some ADT definitions

lint ImproperCTypes: clean up exported static variables

6653984

lint ImproperCTypes: changes to pass CI [...]

f24ff07

- make clippy happy about the changes in rust_codegen_llvm - split a test stderr into 32bit and 64bit - fix some documentation

lint ImproperCTypes: remove the parts about checking value assumptions

a8f55c9

it would be correct to lint on those, but this is deemed too steep a change for now, especially for projects that turn all warnings into errors

lint ImproperCTypes: misc. changes due to review

1d0a9f9

- relaxed the uninhabited types allowed on function return

TEMP COMMIT, to be deleted on rebase: remove i128/u128 warnings

a129537

niacdoial force-pushed the linting-ptrdyn-ffi branch from 2821fb5 to a129537 Compare June 10, 2025 23:29

		// but for some reason one can just go and write function pointers like that:
		// `type Foo = extern "C" fn(::std::ffi::CStr);`

lint ImproperCTypes: overhaul (take 2 of "better handling of indirections") #134697

Are you sure you want to change the base?

lint ImproperCTypes: overhaul (take 2 of "better handling of indirections") #134697

Conversation

niacdoial commented Dec 23, 2024 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niacdoial commented Dec 23, 2024

Uh oh!

niacdoial commented Dec 23, 2024

Uh oh!

workingjubilee commented Dec 24, 2024

Uh oh!

workingjubilee commented Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

workingjubilee commented Jan 4, 2025

Uh oh!

niacdoial commented Jan 5, 2025

Uh oh!

bors commented Jan 15, 2025

Uh oh!

niacdoial commented Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Jan 23, 2025

Uh oh!

This comment has been minimized.

workingjubilee commented Jan 31, 2025

Uh oh!

workingjubilee commented Jan 31, 2025

Uh oh!

workingjubilee commented Feb 20, 2025

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

workingjubilee Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niacdoial commented Feb 27, 2025

Uh oh!

This comment has been minimized.

rustbot commented May 30, 2025

Uh oh!

niacdoial commented May 30, 2025

Uh oh!

niacdoial commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

things that might need further changes

things with no further changes needed IMO

Uh oh!

bors commented May 31, 2025

Uh oh!

niacdoial commented May 31, 2025

Uh oh!

workingjubilee commented Jun 4, 2025

Uh oh!

workingjubilee commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niacdoial commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

niacdoial commented Dec 23, 2024 •

edited by rustbot

Loading

workingjubilee commented Dec 24, 2024 •

edited

Loading

niacdoial commented Jan 18, 2025 •

edited

Loading

niacdoial commented May 31, 2025 •

edited

Loading

workingjubilee commented Jun 4, 2025 •

edited

Loading

niacdoial commented Jun 4, 2025 •

edited

Loading

bjorn3 Jun 6, 2025 •

edited

Loading